🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu

Recommendation: REQUEST_CHANGES — the new firing machinery is correct and well-tested, but the RPC create path produces reminders the scanner can never select, which blocks the cron acceptance criterion end-to-end.

What's solid

The handler, repo due-query/CAS, delivery delegation, self-scheduling, and tests are all sound:

• listDueReminders correctly filters status='active' AND next_run_at IS NOT NULL AND next_run_at <= now joined to a.status='active' — paused agents are filtered at the query and skipped again at delivery.
• advanceReminderAfterFire CAS (status='active' AND next_run_at=<read>) is correct; cron recomputes via getNextRunAt (null → terminal fired), one-shot 'at' flips to fired.
• Deliver-before-advance ordering means a one-shot is never marked fired undelivered.
• Single-threaded processor + reclaimStale recovers the self-scheduling chain after a crash; idempotent enqueueIfMissing prevents duplicate chains.
• Delivery delegates to the existing deliverLongHorizonExternalEvent path → ensureLongHorizonAgentSession → ensureQueryStarted() genuinely wakes the agent.
• All gates green (lint, typecheck, knip, session-guards, db-schema-parity, test-quality); 16 new unit tests pass.

🔴 P1 — RPC-created reminders are dead rows (cron acceptance not met end-to-end)

packages/daemon/src/lib/rpc-handlers/space-long-horizon-agent-handlers.ts:504-513 accepts triggerType: 'at' | 'cron' and cronExpression but never passes nextRunAt to repo.createReminder, so the column defaults to NULL (space-long-horizon-agent-repository.ts:272). The scanner's due-query requires next_run_at IS NOT NULL (...repository.ts:309), so every reminder created via RPC is never selected and never fires — both 'at' and 'cron'.

This is pre-existing (not introduced by this diff), but it directly undermines an acceptance criterion: cron reminders can only be created via this RPC (the MCP create_agent_reminder hardcodes triggerType: 'at', space-agent-tools.ts:1449), so "A cron reminder re-fires on schedule and advances next_run_at" can't be demonstrated end-to-end. The cron advance logic itself is correctly unit-tested in isolation, but the create→fire→advance loop is broken at the create step.

Fix (small): seed nextRunAt in the RPC handler — for 'at' use params.runAt (require it); for 'cron' recompute getNextRunAt(params.cronExpression, params.timezone ?? 'UTC', Date.now()). Or, if the RPC is intentionally out of scope, say so explicitly and file a follow-up — but then the cron acceptance only holds at the unit level.

🟡 P2 — idempotencyKey does not actually dedupe (comment implies it does)

injectLongTermAgentMessage passes the key as the SDK message uuid, but saveUserMessage generates a fresh row PK each call and sdk_messages.uuid has no unique constraint, and MessageQueue.enqueueWithId doesn't dedupe — so a re-delivery with the same key persists+queues a duplicate. The handler comment ("A re-delivery of the same occurrence reuses the key") reads as if the key enforces dedup; the nearby crash-double-fire note contradicts that. This is inherited from the existing external-event delivery path (not a regression), and true double-fire is rare, but please soften the comment to "key is for correlation/tracing, not hard dedup" — or enforce dedup (delivered-occurrence guard) if the name is meant to be load-bearing.

⚪ P3 — nits

• long-horizon-agent-reminder-fire.handler.ts:179 builds the idempotency key from the scanned reminder.nextRunAt while the CAS at :216 uses the re-read fresh.nextRunAt. Equal in practice (single-threaded), but use fresh.nextRunAt for the key for internal consistency.
• Handler :198-201 comment conflates "paused agent" (filtered by the join) with "paused reminder" (no such status exists today). Behavior is correct; the wording could be tighter.

Verdict

New code: correct. Blocking item is the RPC create path (P1) needed to actually satisfy the cron acceptance. Please seed next_run_at there (or explicitly scope to MCP-only) and I'll re-review.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d1cd7f0d5

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 67f62682e8

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b05ef93ff

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 237d6dd021

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu

Recommendation: APPROVE — all prior findings addressed; new hardening is correct; gates and tests green.

Prior findings — resolved

• P1 (RPC next_run_at) — space-long-horizon-agent-handlers.ts:505-526 now seeds nextRunAt ('at'→runAt, required; 'cron'→getNextRunAt after isValidCronExpression, throws on null). The RPC test asserts a cron reminder created via RPC is selected by listDueReminders once due — the cron acceptance criterion now holds end-to-end. ✅
• P2 (idempotency-key comment) — comment now states explicitly the key is for tracing and is not a hard dedupe fence; the real guard is the per-reminder lock + CAS advance. ✅
• P3 (key from fresh.nextRunAt) — fixed. ✅

New hardening — verified correct

• Per-reminder in-process lock (fireReminderSerialized, mirroring longTermAgentFlushes) correctly serializes overlapping scans on the same reminder; the loser re-reads the advanced row and skips (fresh.nextRunAt > now). The overlap test confirms a single delivery. The lock's promise-chain, .catch(() => {}) non-short-circuit, and identity-checked cleanup are all correct.
• Space-lifecycle guard — due-query joins spaces on status='active' AND paused=0 AND stopped=0 (columns exist via migrations 85/87); handler re-checks getSpace before delivery. Repo test covers paused/stopped exclusion.
• Pagination + excludeIds prevents within-scan poison-batch starvation; verified NOT IN with MAX_PER_SCAN=5000 placeholders is well under SQLite's bind limit. Non-silent warn logs on cap/deadline.
• Startup backfill (backfillLongHorizonAgentReminderNextRunAt) is idempotent, non-fatal, NODE_ENV !== 'test'-guarded, and tested for both 'at' and 'cron' legacy rows.
• Cron advance from firedAt = Date.now() correctly avoids a stale scan-now persisting an already-overdue next_run_at.

Accepted follow-ups (non-blocking, P3)

1. ensureLongHorizonAgentSession TOCTOU — the delivery path re-checks space-exists but not paused/stopped, so a stop racing between the handler's recheck and session-ensure can yield ≤1 stray delivery (self-correcting next scan). Agreed this belongs in the shared delivery path the parallel wake-context task is consolidating — fixing it here is exactly the merge conflict the task description warns against. Track as a follow-up.
2. Cross-scan durable backoff for persistent-failure batches — mitigated today by within-scan excludeIds + the active-agent due-query filter. The constraint (cron stays on-schedule; paused-agent reminders fire on resume) makes a naive backoff risky; a dedicated retry column is the right shape later.
3. Nit: the "skips when the space is stopped between select and fire" handler test exercises the !delivered skip path (the deliver hook stops the space during delivery), not the handler's getSpace recheck itself — the repo-level due-query test is what actually covers the stopped-space filter. Optional: tighten the test name.

Verdict

New code and all fixes are correct and well-tested (36 RPC + 23 handler + repo/storage tests; lint/typecheck/knip/session-guards/db-schema-parity/test-quality all green). Shipping this; the three items above are tracked follow-ups, not blockers.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 237d6dd021

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu

Recommendation: REQUEST_CHANGES — Codex's fresh re-review (20:31, on the true head) surfaced two real P2s in the new scanner code that my first pass under-rated. (Its earlier inline review was stale and is disregarded — RPC next_run_at, space filter, overlap lock, and cron-fire-time are all addressed.) The firing machinery itself remains correct; these are robustness/correctness gaps in the scan loop and retry path.

🟡 P2 — Re-check the scan deadline inside the per-reminder loop

long-horizon-agent-reminder-fire.handler.ts:127 checks SCAN_DEADLINE_MS only before fetching each page. Inside the for loop each await fireReminderSerialized(...) (:138) can block up to the MessageQueue.enqueueWithId 30s timeout. A single page of 100 stuck sessions = ~50 min on one worker; with maxConcurrent=5, five poison pages consume every worker and stall graceful shutdown. Fix: check the deadline (and optionally MAX_PER_SCAN) after each delivery inside the inner loop and break out.

🟡 P2 — Persisted-message amplification on a failed delivery

injectLongTermAgentMessage calls saveUserMessage(...) (persisting an enqueued sdk_messages row) before awaiting session.messageQueue.enqueueWithId(...). On the queue's 30s timeout the call throws, the handler catches it and returns failed without advancing the reminder, so the next scan re-selects the occurrence, re-injects, and persists another row (new PK, same uuid — sdk_messages.uuid is non-unique). When the session later recovers, replayPendingMessagesForImmediateMode replays every persisted row → many duplicate reminders. Fix (pick one): (a) make the shared inject idempotent on the message uuid (unique constraint / check-then-insert — coordinate with the wake-context task that's consolidating this path); (b) handler-level guard so an occurrence already persisted-and-pending isn't re-injected. Blanket "advance on throw" is not safe — it would silently drop one-shot 'at' reminders on a transient failure.

⚪ P3 — Backfill reminders owned by paused agents

space-long-horizon-agent-repository.ts:376 listActiveRemindersWithNullNextRunAt filters a.status='active', so a legacy reminder whose owner is paused at upgrade stays NULL and never fires even after reactivation. Fix: drop the agent-status filter (let the due-query gate at fire time); archived-agent rows get backfilled but never fire, which is harmless.

Accepted non-blocking follow-ups (unchanged)

• TOCTOU in ensureLongHorizonAgentSession (shared path) — deferred to the parallel wake-context task.
• Cross-scan durable backoff for persistent-failure batches — mitigated today by within-scan excludeIds.
• No LiveQuery/event publish on fire — likely a separate frontend task.

Verdict

The P2 deadline check is a trivial, clearly-in-scope fix; the amplification needs a design decision but is a real duplicate-message risk exposed by the scanner's retry loop. Please address the two P2s (and the P3 backfill while you're in there) and I'll re-review. Holding the QA handoff until then.

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE

Round-3 re-review of d0c0ae7d6 (per-reminder deadline, persisted-message idempotency guard, backfill all agents). All three changes are correctly implemented and tested; no P0–P3 findings remain.

Verified this round:

• Per-reminder deadline (handler.ts:144): the SCAN_DEADLINE_MS budget is now checked before each delivery inside the inner for-loop, with a labeled break scanLoop that exits both loops. attempted.add() placement is correct — a deadline-deferred reminder isn't re-selected within the same scan, and remains due for the next tick (intended "deferred, not lost" semantics).
• Persisted-message guard (handler.ts:269 + app.ts:957): on retry, isOccurrencePersisted looks up the occurrence's uuid in the owning LH-agent session's sdk_messages (enqueued/consumed); when present it skips re-inject and advances. Confirmed end-to-end consistency — deliver persists with uuid === idempotencyKey, and app.ts derives the same sessionId (longTermAgentSessionId) the deliver path persists into. The skip+advance (vs delete-on-failure) choice is sound: a prior attempt's persisted enqueued row has three independent drain paths (startup replay via replayPendingMessagesForImmediateMode, turn-end acknowledge fallback, yield-time consumption), so the occurrence reaches the agent exactly once rather than being orphaned or duplicated. The enqueued/consumed coverage is correct for this path — deferred is manual-mode-only and unreachable for auto-injected reminder messages.
• Backfill filter drop (repository.ts:374): listActiveRemindersWithNullNextRunAt now selects all active reminders regardless of agent status, so paused-agent reminders get next_run_at seeded and fire when the agent reactivates. No backfill-then-immediate-fire risk while paused: listDueReminders still requires a.status='active', so a paused-agent reminder is never selected until the agent is active again. Past-time next_run_at for an 'at' reminder firing on resume is the intended one-shot semantics.
• Tests: 61 pass / 0 fail across the three affected test files (27 unit tests total for the feature). Full quality gate green (lint, typecheck, knip, session-guards, space-task-handler-tests, db-schema-parity, test-quality).

All prior review threads from rounds 1–2 are resolved. Re-triggering Codex for a fresh verdict on this head.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0c0ae7d66

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES

Round-3 re-review of d0c0ae7d6. The three round-3 changes (per-reminder deadline, persisted-message guard, backfill-all-agents) are correctly implemented and tested, and the full gate is green. However, Codex's fresh review (21:00:06Z) raised two P2s that I judge to be real defects, so I'm requesting changes rather than handing off. Detail and recommended fixes below.

P2 #1 — Guard marks a one-shot fired without confirmed delivery (violates this handler's own invariant)

long-horizon-agent-reminder-fire.handler.ts:269 — the isOccurrencePersisted guard treats a persisted row (enqueued or consumed) as delivered and advances. But this file's header explicitly states the contract (lines 32-36):

"delivery happens before the advance so a one-shot reminder is never marked fired without having actually been delivered."

"Persisted as enqueued" is not "delivered." If enqueueWithId timed out because the SDK is stuck, the row sits enqueued and is only drained on session restart or a turn-end the stuck SDK never reaches — yet a one-shot reminder is advanced to status='fired'. That directly violates the stated invariant.

Recommended fix (small): make the occurrence lookup tri-state and branch on it:

• consumed → advance (the message reached the agent — invariant holds).
• enqueued → skip without advancing (leave the reminder due; the SDK drains the persisted row via its normal turn-end/yield/startup paths; the scanner re-selects it next tick and re-checks). Crucially, do not call deliver here, so there is no re-inject → no amplification, and the reminder is never marked fired until truly consumed.
• absent → deliver as today.

This preserves both the invariant and the amplification fix. (The "retry every scan until consumed" cost is one indexed getMessageByStatusAndUuid lookup per stuck reminder per 30s tick — negligible, and bounded by MAX_PER_SCAN.) If you prefer, the alternative is to make the delivery path itself idempotent on the uuid (insert-or-requeue) and drop the scanner-side guard entirely — but the tri-state guard is the smaller change.

P2 #2 — Lifecycle TOCTOU: no fence immediately before injectLongTermAgentMessage

space-runtime-service.ts:366-382 — deliverLongHorizonExternalEvent checks the agent is status='active' (line 373) but does not re-check space.paused/space.stopped after the ensureLongHorizonAgentSession await (line 376) and before injectLongTermAgentMessage (line 378). The scanner's space-lifecycle precheck (handler.ts:251-254) runs before that await, so a space paused/stopped during session prep still receives the inject — recreating/waking a session an operator just stopped. This is the TOCTOU you flagged as deferred in round 2; Codex has now independently re-raised it and the fix is cheap, so I think it's worth pulling into scope.

Recommended fix: after ensureLongHorizonAgentSession and before injectLongTermAgentMessage, re-read the space (and the agent) and return { delivered: false } if the space is no longer active / is paused / is stopped, or the agent is no longer active.

What's good

• Deadline-per-reminder (labeled break scanLoop, checked before each delivery) — correct.
• Backfill-all-agents (paused-agent reminders seeded, gated at fire by the a.status='active' join) — correct, tested.
• 61 tests pass / 0 fail; full quality gate green.

Once P2 #1 (and ideally P2 #2) are addressed, re-request review and I'll re-trigger Codex. Not handing off to QA until the guard's invariant is restored.

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE

Round-4 re-review of 0d060d548. Both P2s from the prior round are correctly addressed; no P0–P3 findings remain.

P2 #1 — persisted-message guard (deliver-before-advance invariant) — fixed. isOccurrencePersisted (boolean) is replaced by a tri-state getOccurrenceDeliveryState returning absent | enqueued | consumed, branching in fireReminder:

• consumed → advance without re-injecting (the occurrence reached the agent — invariant holds).
• enqueued → return 'skipped' before any advance or re-inject — the reminder stays due, the scanner re-checks next tick, and the persisted row is drained to consumed by the SDK's turn-end auto-send (sendEnqueuedMessagesOnTurnEnd; sdk_messages send_status='enqueued' is the auto-send set). No duplicate row, and a one-shot is never marked fired before delivery.
• absent → deliver as usual.

Verified the control flow (the enqueued early-return exits fireReminder before the CAS-advance block) and the app.ts wiring (probes consumed before enqueued, so a consumed row isn't misclassified). Two targeted tests pin the behavior: consumed asserts deliver.calls=0 + fired=1; enqueued asserts deliver.calls=0 + fired=0 + skipped=1 + reminder stays active/due.

P2 #2 — lifecycle TOCTOU before inject — fixed. deliverLongHorizonExternalEvent now re-reads space (status==='active' && !paused && !stopped) and agent (status==='active') after the ensureLongHorizonAgentSession await and before injectLongTermAgentMessage, returning {delivered:false} otherwise. Closes the window the scanner's pre-await precheck left open, for both reminders and external events. spaceManager.getSpace is an established awaited call in the same file (e.g. ensureLongHorizonAgentSession), and typecheck confirms the paused/stopped field access.

Coverage note (non-blocking): the inject-path lifecycle recheck has no dedicated SpaceRuntimeService unit test (no harness in the suite). It's exercised indirectly by the external-events tests in the 5-space-runtime-a shard (390 pass) plus the handler-side delivered:false→skip test, and the code is simple and typechecked — acceptable as-is; a dedicated harness would be a nice future pin but isn't required.

Verification: 62 pass / 0 fail across the three reminder test files; 5-space-runtime-a 390 pass / 0 fail; full quality gate green (lint, typecheck, knip, session-guards, space-task-handler-tests, db-schema-parity, test-quality). All prior review threads resolved. Re-triggering Codex for a fresh +1 on this head.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d060d5486

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES

Round-4 re-review of 0d060d548. Your round-4 fixes for the prior P2s are correct in themselves (tri-state guard logic, lifecycle-recheck placement both verified). But Codex's fresh review (21:31:49Z) surfaced three deeper issues, all of which I've independently confirmed by tracing the code. The headline is a P1: a single stuck SDK delivery can deadlock the job processor. Detail and fixes below.

P1 — Unbounded delivery wait can deadlock the job processor (must fix)

enqueueWithId has no hard upper bound once the SDK has pulled the message. Its 30s timeout only rejects messages still in the in-memory queue (message-queue.ts:133 — indexOf(queuedMessage) guard; if already shifted out at :180, the timeout is a no-op), and the promise resolves only via onSent, which the wrapper runs after yield message (query-runner.ts:1603-1604) — so if the SDK's for await never resumes or terminates (stuck subprocess), onSent never fires and enqueueWithId never settles.

The handler awaits this inside the per-reminder lock (fireReminderSerialized → await deliver). SCAN_DEADLINE_MS is checked only before entering the lock, not during the await, so one hung delivery pins that lock and that scan job indefinitely. Each scan pre-enqueues its successor at the top of the run, so successive scans (30s apart) each grab a JobQueueProcessor slot and hang on the same reminder — with maxConcurrent=5, all slots saturate and unrelated queues (memory-consolidation, task-schedule-fire, …) halt.

Fix (localized): bound the delivery wait. Wrap await deliver(...) (or the lock hold in fireReminderSerialized) in a Promise.race against a per-delivery deadline — e.g. ~35s, just past the message-queue's 30s in-queue timeout. On timeout, release the lock and return 'failed'; the reminder retries next scan, each attempt bounded. Handler-level is preferable to touching MessageQueue, whose 30s-while-in-queue behavior is load-bearing for legitimately-slow SDK startup. (STARTUP_TIMEOUT_MS / the stale-state detector recover the session at a higher layer but do not settle the orphaned enqueueWithId promise, so they don't save the slot.)

P2 — consumed is not a reliable "delivered to model" signal (the invariant still has a hole)

The tri-state guard advances on send_status='consumed', but consumed is overloaded. Three of the four write-sites are genuine SDK-yield transitions (consumePersistedUserMessage sdk-message-handler.ts:357; handleMessageYielded :454/:492). The fourth — acknowledgeOldestQueuedUserOnTurnEnd (sdk-message-handler.ts:399-430, triggered at turn end via handleResultMessage:891-894) — is a cleanup that marks all leftover enqueued user rows consumed when the SDK ends a turn without replaying/yielding them (the model never saw them). So a one-shot can be advanced to fired after the cleanup consumed its row — re-violating "a one-shot is never marked fired without delivery," the very invariant round-4 restored. The guard cannot distinguish genuine-yield consumed from cleanup consumed.

Fix options: (a) split the terminal so cleanup-consumed is distinct (e.g. a column/flag written only by the cleanup) and have the guard treat it like enqueued (defer); (b) have acknowledgeOldestQueuedUserOnTurnEnd skip reminder-occurrence rows (uuid prefix reminder:); (c) key the guard on a delivery-receipt written only from handleMessageYielded. For a nag system, a defensible lighter alternative is to relax the invariant (accept consumed ≈ "handed to the SDK pipeline") and rewrite the handler doc (lines 32-40) + guard doc to match — but then the "never marked fired without delivery" promise must be honestly restated. Pick one; the current code+doc are inconsistent.

P2 — ensureLongHorizonAgentSession side-effects fire before the lifecycle gate

The post-await recheck (round-4) closes the inject TOCTOU, but ensureLongHorizonAgentSession runs first and performs ungated side effects: session creation (DB row + cache + session.created), config refresh + resetQuery({restartQuery:true}) (tears down/restarts the live query), agent-row update, metadata write, and MCP attachment (space-agent-tools/agent-memory/db-query, allocating a fresh db-query server) — none gated by space active/paused/stopped. So pausing/stopping a space just as a reminder fires still triggers session creation / query-restart / MCP-attach before deliverLongHorizonExternalEvent returns delivered:false.

Fix (small): add the same lifecycle gate before await this.ensureLongHorizonAgentSession(...) (line 376), and keep the existing post-await recheck for the residual race during the await.

Prioritization

P1 is blocking (job-processor availability). P2 #2 is a small, clean add. P2 #1 is the design call — pick a fix or relax-and-document, but reconcile code with the doc. Once the P1 is bounded you may also find some of the layered defenses (lock + tri-state guard) can simplify — worth a look, not required.

Verification this round: all three findings confirmed by reading message-queue.ts, query-runner.ts, sdk-message-handler.ts, and ensureLongHorizonAgentSession directly (citations above); 62 reminder tests + 390 space-runtime-a tests pass; full gate green. Not handing off to QA — findings remain and Codex has not +1'd this head.

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE

Round-5 re-review of 680e0a9a9. All three prior findings (P1 + 2×P2) are properly resolved; no P0–P3 findings remain.

P1 — bounded delivery (must-fix) — fixed. await deliver(...) is wrapped in withTimeout(..., DELIVERY_TIMEOUT_MS=35s) (handler.ts); on timeout it rejects → the catch returns 'failed' → the fireReminderSerialized finally releases the per-reminder lock and the scan job completes, so a wedged SDK can no longer pin a slot or, across pre-enqueued scans, saturate maxConcurrent and stall unrelated queues. Verified the helper's correctness: the abandoned deliver promise has .then(value,err) handlers attached, so its eventual settle is a no-op on the already-settled outer promise — no unhandled rejection, no double-settle, and clearTimeout runs on settle. Also confirmed no growing leak / retry amplification: on the next scan getOccurrenceDeliveryState returns 'enqueued' (the prior attempt persisted the row) → defer, no new deliver/enqueueWithId, so only one hung promise per stuck reminder (freed on session recovery). And each scan is bounded (~35s for one stuck delivery, then the 20s SCAN_DEADLINE_MS breaks the loop), so at most ~2 scans overlap against maxConcurrent=5. New test "bounds a stuck delivery with a per-call timeout" uses a never-settling deliver + 50ms injection and asserts completion < 2s — pins the fix precisely.

P2 — pre-await lifecycle gate — fixed. deliverLongHorizonExternalEvent now fetches the space and bails (status!=='active' || paused || stopped) before await ensureLongHorizonAgentSession, so session creation / resetQuery / agent-row update / MCP-attach no longer fire for a non-deliverable space. The post-await recheck is retained for the during-await race — correct double-fence.

P2 — consumed overload — reconciled (relax-and-document). Rather than touch the shared SDK message-handling path, the header now honestly states consumed is the "handed to the SDK pipeline" bar for a nag, explicitly documents the acknowledgeOldestQueuedUserOnTurnEnd cleanup residual (a one-shot may advance with the message in session history but not yet yielded this turn), and notes how to tighten later (split cleanup-consumed / key off a yield receipt). Code and doc are now consistent — the over-claim is gone. Defer-on-enqueued preserved, and the turn-end/startup re-drain to consumed (then advance) is sound.

Refactor: fireReminderSerialized/fireReminder now take the deps bag — cleaner than the widening param lists; destructuring is correct.

Verification: 63 pass / 0 fail across the three reminder test files (incl. the new timeout test); 5-space-runtime-a 390 pass / 0 fail; full quality gate green (lint, typecheck, knip, session-guards, space-task-handler-tests, db-schema-parity, test-quality). All 20 review threads resolved. Re-triggering Codex for a fresh +1 on this head.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 680e0a9a9d

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 680e0a9a9d

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 680e0a9a9d

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES — correcting my premature round-5 APPROVE (review #4782669958). I approved before Codex finished reviewing head 680e0a9a9; Codex then posted three reviews (#4782684937, #4782706627, #4782723595) whose findings I've now independently verified. Two are P1s I missed — both stem from the round-5 withTimeout fix solving slot-pinning while leaving two gaps: (1) amplification when delivery hangs before the occurrence row is persisted, and (2) scan starvation when one reminder's delivery exceeds the scan budget.

P1 #1 — Amplification: a timed-out delivery that hangs before saveUserMessage is not reserved, so the next scan fires a parallel delivery

long-horizon-agent-reminder-fire.handler.ts:300-336, space-runtime-service.ts:395-411,558-560

The tri-state guard keys off the persisted sdk_messages row: getOccurrenceDeliveryState returns 'enqueued'/'consumed' only once saveUserMessage has run. But deliver can hang in ensureLongHorizonAgentSession (resetQuery({restartQuery:true}) at space-runtime-service.ts:628) or in injectLongTermAgentMessage's own ensureQueryStarted() (:558) — both before saveUserMessage (:559). In that window no row exists, so on timeout the catch returns 'failed', fireReminderSerialized's finally releases the per-reminder lock, and the abandoned deliver() keeps running. The next scan (30s later) sees 'absent' and starts a second deliver() in parallel; successive scans pile up more. When startup eventually recovers, each in-flight attempt reaches saveUserMessage + enqueueWithId with the same idempotencyKey UUID, and sdk_messages has no uniqueness constraint on it → the agent receives the reminder N times.

This is the case my round-5 reasoning ("next scan sees 'enqueued' → defer, one hung promise") got wrong — that only holds once saveUserMessage has persisted the row.

Fix direction: reserve the occurrence before awaiting delivery so the probe sees it as in-flight regardless of where delivery hangs — e.g. an in-process delivering set keyed by idempotencyKey that getOccurrenceDeliveryState consults (returning 'enqueued'), cleared only when deliver actually settles; or persist an idempotency-marker row up front; or hold the per-reminder lock until the abandoned promise settles rather than on timeout.

P1 #2 — Starvation: one poison reminder blocks every later reminder indefinitely

long-horizon-agent-reminder-fire.handler.ts:184-212

DELIVERY_TIMEOUT_MS (35s) > SCAN_DEADLINE_MS (20s), and the budget check is at the top of the per-reminder loop (:191). So a single reminder whose delivery times out consumes the entire scan budget — the deadline break fires before the second reminder in the page is ever attempted. The attempted set is scan-local (discarded each run), and listDueReminders orders by next_run_at, so the next scan re-selects the same earliest, still-due, never-advanced poison reminder first. One unhealthy agent (e.g. a session whose resetQuery never settles) therefore prevents all later reminders from ever firing — the opposite of the "starvation safety" the header claims.

Fix direction: don't let one slow/failed attempt eat the whole scan. Either (a) a per-reminder cost cap smaller than the remaining budget so cheap reminders after a poison one still run, and/or (b) a cross-scan failure cooldown/skip-cursor (persist last_failed_at, defer N ticks) so a repeatedly-failing reminder stops being selected first. MAX_PER_SCAN/deadline currently bound shutdown latency but not fairness.

P2 — Residual TOCTOU between the post-await gate and saveUserMessage

space-runtime-service.ts:402-411,558

The lifecycle gate runs pre-await (:385) and post-ensureLongHorizonAgentSession-await (:402), but injectLongTermAgentMessage then awaits ensureQueryStarted() (:558) — a long await — with no lifecycle recheck before saveUserMessage/enqueueWithId. A pause/stop that lands during query startup still gets the injection, so the header's "a paused/stopped space never has a reminder injected" is slightly over-stated. One more getSpace recheck immediately before saveUserMessage (or after ensureQueryStarted) closes it.

P2 — Backfill fires malformed reminders instead of skipping them

long-horizon-agent-reminder-fire.handler.ts:406-410

triggerType === 'cron' with a falsy cronExpression falls through to reminder.runAt ?? now (a cron reminder shouldn't use runAt), and an 'at' reminder with null runAt becomes now. Both turn a previously-inert row into an immediate fire+terminal-fired on upgrade. Skip at rows lacking runAt and cron rows lacking a valid expression (leave them for an operator).

P2 (low) — Sub-30s cron schedules silently drop occurrences

long-horizon-agent-reminder-fire.handler.ts:64,356 — NEXT_SCAN_DELAY_MS=30s can't honor a 6-field cron with a sub-30s interval, and getNextRunAt(..., firedAt) computes forward from the (late) fire time, permanently skipping the intervening runs. Either reject/cap reminder cadence at scan precision, or schedule the next scan at the earliest pending occurrence. Low priority (pathological cadence for a nag), but the cron util accepts 6-field expressions so it's a supported-but-broken input.

Dismissed — "Defer paused-space external events instead of exhausting retries" (Codex P1, #4782684937)

I traced this and it is not a regression: the pre-await lifecycle gate is behind options.gateSpaceLifecycle === true, set only on deliverLongHorizonAgentReminder (the new reminder path, space-runtime-service.ts:431-437). The external-event path (deliverLongHorizonExternalEvent with no options, wired at :298) is byte-for-byte unchanged by this PR — no new delivered:false returns for a paused space on that route. The reminder path returning delivered:false on pause is intentional and correct (the scanner skips and retries next scan after resume). The "onSpaceResumed can't reconstruct LH rows" premise is also incorrect: requeuePersistedPendingDeliveries (space-runtime.ts:5065-5123) has a dedicated LH branch that treats delivery.taskId as a subscription ID and re-dispatches. No change needed here — flagging so it isn't chased.

Nit (follow-up, not blocking) — No client event on active→fired

space-long-horizon-agent-repository.ts:340-364, SpaceLongHorizonAgents.tsx:478-495 — a fired one-shot updates the row via direct SQL with no pub/sub event, so the web badge (refetch only on [agents.length, spaceId]) stays stale. Real, but the sibling reminder paths (create/delete RPC, create tool) are equally silent — there's no reminder event channel at all — so this PR matches the status quo rather than introducing a regression. Best handled as a dedicated change adding reminderCreated/Updated events across all reminder write paths.

Verification

Local: 63 pass/0 fail across the three reminder test files; 5-space-runtime-a 390/0; full quality gate green (lint, typecheck, knip, session-guards, space-task-handler-tests, db-schema-parity, test-quality). CI 1-core (getProviderService) and 4-space-migrations-a are flakes — both pass locally and are identical on dev's latest commit; this PR touches no migrations/providers/schema.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf6a24e303

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES — one new P0 introduced by the round-6 fix. The two P1s and the backfill P2 from round 6 are genuinely fixed and verified, and the dismissed external-event concern is now correctly reflected in the code (gate is reminder-only). But the in-flight tracker's delivery.finally(cb) is a bare fire-and-forget whose returned promise is unhandled when deliver rejects — and the daemon treats unhandled rejections as fatal (process.exit(1)), so a routine, recoverable reminder-delivery failure crashes the entire daemon.

P0 — delivery.finally(cb) crashes the daemon when deliver rejects

long-horizon-agent-reminder-fire.handler.ts:344

The handler already catches deliver's rejection (the try/catch around withTimeout → 'failed'). But delivery.finally(cb) returns a separate promise that re-throws the rejection, and that returned promise is neither captured nor awaited. I verified this empirically (Bun repro): with withTimeout's .then(value, err) attached — so delivery itself is handled — a rejecting delivery still surfaces !!! UNHANDLED REJECTION DETECTED from the discarded .finally() result.

deliver rejects on multiple reachable paths: ensureLongHorizonAgentSession → refreshLongHorizonAgentSessionConfig throws on resetQuery failure (space-runtime-service.ts:628-631); createSession failures rethrow (:655-658); injectLongTermAgentMessage can throw on saveUserMessage/enqueueWithId. Any of those → unhandled rejection.

And the daemon treats that as fatal: packages/daemon/main.ts:19-31 (and packages/cli/prod-entry.ts:25-29) register process.on('unhandledRejection', … → process.exit(1)) — active in both dev and prod (main.ts is the daemon entry; createDaemonApp is called from dev-server/prod-server/prod-server-embedded). So a single reminder whose delivery throws — exactly the recoverable case the handler is designed to treat as 'failed' and retry next scan — instead crashes the whole daemon, and evolution-log-evidence-service classifies it as a runtime_crash.

The sibling .finally() calls in the codebase are safe by construction: channel-router.ts:1507 is fn().then(resolve, reject).finally(release) — the preceding .then(resolve, reject) neutralises the rejection so .finally() sits on a fulfilled promise; session-manager.ts:222 and github-event-extension.ts:706 capture/track the result. This is the only bare, result-discarding .finally() on a rejecting promise.

Fix (one line), matching channel-router's idiom — use a paired handler so the derived promise fulfils either way:

const clearInFlight = () => {
  if (reminderDeliveriesInFlight.get(fresh.id) === delivery) {
    reminderDeliveriesInFlight.delete(fresh.id);
  }
};
delivery.then(clearInFlight, clearInFlight);

(or delivery.finally(clearInFlight).catch(() => {})). Add a test where deliver rejects and assert the handler returns 'failed' with no unhandled rejection — none of the current tests exercise the reject path (they use a never-settling deliver), which is why this slipped through.

Verified fixed (round 6)

• P1 amplification — reminderDeliveriesInFlight (keyed on reminder id, cleared on settle) makes the next scan skip a still-in-flight delivery instead of reading 'absent' and stacking a parallel deliver(). The "does not stack a second delivery while one is in flight" test pins it (scan 2 deliver.calls === 0). Keying on id is equivalent to idempotencyKey here since only one occurrence is due at a time.
• P1 starvation — the same tracker skips a repeatedly-timing-out earliest reminder on subsequent scans (fast has() check, no 35s cost), so later reminders get their turn. Bounded to ~N scans of delay for N stuck reminders, not indefinite.
• P2 backfill — cron-without-expression → null (skip); at-without-runAt → null (skip). The "skips unschedulable reminders" test covers both.
• Dismissed external-event concern — the gate is now correctly behind options.gateSpaceLifecycle (default false); the external-event path (space-runtime-service.ts:298) is ungated and unchanged; reminders pass { gateSpaceLifecycle: true }. The doc comment records why.

Accepted residuals (no change required this PR)

• P2 TOCTOU (injectLongTermAgentMessage's ensureQueryStarted await after the gates) — narrow window; the wedged case is bounded by DELIVERY_TIMEOUT_MS; fully closing it would re-gate external events. Agreed: thread a reminder-only flag through inject as a follow-up.
• P2 sub-30s cron — documenting the ~30s scan floor is fine; sub-30s reminders are nonsensical for a nag. Creation-time rejection is a reasonable follow-up.
• P3 fired-state client event — matches the status quo (no reminder event channel exists); dedicated follow-up.

Verification

18 pass / 0 fail on the handler test file (incl. the new in-flight + backfill tests). The P0 is in a path none of the existing tests exercise; Bun repro confirms the unhandled rejection and main.ts:19 confirms the crash.

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE — round-7 P0 fixed and verified.

The bare delivery.finally(cb) is now delivery.then(clearInFlight, clearInFlight) (long-horizon-agent-reminder-fire.handler.ts:344-352). The paired handler runs cleanup on both settle paths, and because the reject handler returns undefined rather than re-throwing, the derived promise fulfils either way — a rejecting deliver no longer surfaces as an unhandled rejection. The handler's existing try/catch around withTimeout still catches the rejection as 'failed'; the orphaned unhandled promise that crashed the daemon (main.ts:19 → process.exit(1), active dev+prod) is gone. The in-flight tracker still clears correctly on both fulfil and reject, so the round-6 amplification/starvation behaviour is intact. Comment is accurate.

Verification (empirical, not suite-trust — per the process note): the new test "a rejecting deliver returns failed without an unhandled rejection" registers a real process.on('unhandledRejection') listener, uses a deliver that throws, and asserts failed === 1 AND rejections === [] after draining — exactly the path the prior suite missed (it only used never-settling / resolving delivers). Coder confirmed it fails against the .finally version before re-applying the fix. 19 pass / 0 fail on the handler file.

Cumulative status (round 8, head 63bc5e367): every finding from rounds 3–7 is resolved —

• P1 amplification (in-flight delivery tracker; next scan skips a still-in-flight occurrence)
• P1 starvation (the same tracker fast-skips a repeatedly-timing-out earliest reminder on subsequent scans)
• P2 backfill (cron-no-expression / at-no-runAt → null, skipped not fired)
• P0 finally-crash (this round)

Accepted residuals, documented in-code: P2 TOCTOU in injectLongTermAgentMessage's ensureQueryStarted await (narrow window; wedged case bounded by DELIVERY_TIMEOUT_MS); P2 sub-30s cron (~30s scan floor, nonsensical for a nag); P3 fired-state client event (follow-up; matches the status quo — no reminder event channel exists). Dismissed external-event concern correctly reflected (gate behind gateSpaceLifecycle, reminder-only; external-event path :298 unchanged).

Triggering Codex for a fresh +1 on this head; QA handoff once it clears.